Finalize 1.9.3 changelog with Retry-After hardening details#69
Finalize 1.9.3 changelog with Retry-After hardening details#69eXPerience83 merged 1 commit into1.9.2from
Conversation
Summary of ChangesHello @eXPerience83, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of API interactions and improves data parsing flexibility. It specifically hardens the handling of HTTP 429 responses by validating Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47af711458
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if math.isfinite(parsed) and parsed > 0: | ||
| return parsed |
There was a problem hiding this comment.
The new parsed > 0 check treats Retry-After: 0 as invalid and forces the 2-second fallback, but a zero delay is a valid throttling response meaning "retry immediately". In environments where the upstream sends 0 during transient rate limits, this adds an unnecessary fixed delay on every retry and slows recovery compared to the previous behavior.
Useful? React with 👍 / 👎.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the hardening of HTTP 429 backoff by validating Retry-After header values and clamping retry sleep to a safe bounded range. It also improves color channel validation by accepting numeric-string RGB values and ignoring invalid strings without crashing. The CHANGELOG.md has been updated to reflect these changes, and comprehensive tests have been added to cover the new validation logic. The changes enhance the robustness and reliability of the integration, aligning with best practices for defensive input validation.
| delay = delay + random.uniform(0.0, 0.4) | ||
| delay = max(0.0, min(delay, 5.0)) |
There was a problem hiding this comment.
The clamping logic for the retry delay has been improved by applying max(0.0, min(delay, 5.0)) after adding jitter. This ensures the delay is always within the safe bounded range of 0.0 to 5.0 seconds, regardless of the Retry-After value or jitter. This is a critical improvement for stability.
References
- Defensively validate and normalize user input even when it is expected to be valid from UI controls, as it can be malformed due to persisted data or external integrations.
| @pytest.mark.parametrize( | ||
| ("retry_after", "now"), | ||
| [ | ||
| ("-10", None), | ||
| ("nan", None), | ||
| ("inf", None), | ||
| ( | ||
| "Wed, 10 Dec 2025 12:00:00 GMT", | ||
| datetime.datetime(2025, 12, 10, 12, 0, 5, tzinfo=datetime.UTC), | ||
| ), | ||
| ], | ||
| ) | ||
| def test_coordinator_retry_after_invalid_values_use_safe_default( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| retry_after: str, | ||
| now: datetime.datetime | None, | ||
| ) -> None: | ||
| """Invalid Retry-After values should fall back to a safe finite delay.""" | ||
|
|
||
| session = SequenceSession( | ||
| [ | ||
| ResponseSpec( | ||
| status=429, | ||
| payload={"error": {"message": "Quota exceeded"}}, | ||
| headers={"Retry-After": retry_after}, | ||
| ), | ||
| ResponseSpec( | ||
| status=429, | ||
| payload={"error": {"message": "Quota exceeded"}}, | ||
| headers={"Retry-After": retry_after}, | ||
| ), | ||
| ] | ||
| ) | ||
| delays: list[float] = [] | ||
|
|
||
| async def _fast_sleep(delay: float) -> None: | ||
| assert isinstance(delay, float) | ||
| assert delay == delay | ||
| assert delay != float("inf") | ||
| assert delay != float("-inf") | ||
| delays.append(delay) | ||
|
|
||
| monkeypatch.setattr(client_mod.asyncio, "sleep", _fast_sleep) | ||
| monkeypatch.setattr(client_mod.random, "uniform", lambda *_args, **_kwargs: 0.0) | ||
| if now is not None: | ||
| monkeypatch.setattr(client_mod.dt_util, "utcnow", lambda: now) | ||
|
|
||
| client = client_mod.GooglePollenApiClient(session, "test") | ||
|
|
||
| loop = asyncio.new_event_loop() | ||
| hass = DummyHass(loop) | ||
| coordinator = coordinator_mod.PollenDataUpdateCoordinator( | ||
| hass=hass, | ||
| api_key="test", | ||
| lat=1.0, | ||
| lon=2.0, | ||
| hours=12, | ||
| language=None, | ||
| entry_id="entry", | ||
| forecast_days=1, | ||
| create_d1=False, | ||
| create_d2=False, | ||
| client=client, | ||
| ) | ||
|
|
||
| try: | ||
| with pytest.raises(client_mod.UpdateFailed, match="Quota exceeded"): | ||
| loop.run_until_complete(coordinator._async_update_data()) | ||
| finally: | ||
| loop.close() | ||
|
|
||
| assert session.calls == 2 | ||
| assert delays == [2.0] |
There was a problem hiding this comment.
The new parameterized test test_coordinator_retry_after_invalid_values_use_safe_default provides excellent coverage for various invalid Retry-After values, including negative, NaN, infinity, and stale date-based values. This ensures that the system consistently falls back to a safe default delay, significantly improving the reliability of the backoff mechanism.
References
- Defensively validate and normalize user input even when it is expected to be valid from UI controls, as it can be malformed due to persisted data or external integrations.
| try: | ||
| return float(retry_after_raw) | ||
| parsed = float(retry_after_raw) | ||
| if math.isfinite(parsed) and parsed > 0: | ||
| return parsed | ||
| return 2.0 |
There was a problem hiding this comment.
The _parse_retry_after function now correctly handles non-finite and negative float values for Retry-After by returning a default of 2.0. This is a good defensive programming practice to prevent unexpected behavior from malformed or malicious Retry-After headers.
References
- Defensively validate and normalize user input even when it is expected to be valid from UI controls, as it can be malformed due to persisted data or external integrations.
| if retry_at is not None: | ||
| delay = (retry_at - dt_util.utcnow()).total_seconds() | ||
| if delay > 0: | ||
| if math.isfinite(delay) and delay > 0: |
There was a problem hiding this comment.
Adding math.isfinite(delay) to the condition for date-based Retry-After values ensures that infinite or NaN delays are also handled gracefully, preventing potential issues with asyncio.sleep.
References
- Defensively validate and normalize user input even when it is expected to be valid from UI controls, as it can be malformed due to persisted data or external integrations.
| r = _normalize_channel(color.get("red")) | ||
| g = _normalize_channel(color.get("green")) | ||
| b = _normalize_channel(color.get("blue")) |
There was a problem hiding this comment.
The removal of the has_any_channel check simplifies the logic in _rgb_from_api. The _normalize_channel function is now solely responsible for validating and normalizing individual color channels, making the code cleaner and more focused.
| def test_coordinator_accepts_numeric_string_color_channels() -> None: | ||
| """Numeric string channels should be normalized into RGB/hex values.""" | ||
|
|
||
| payload = { | ||
| "dailyInfo": [ | ||
| { | ||
| "date": {"year": 2025, "month": 7, "day": 1}, | ||
| "pollenTypeInfo": [ | ||
| { | ||
| "code": "GRASS", | ||
| "displayName": "Grass", | ||
| "indexInfo": { | ||
| "value": 1, | ||
| "category": "LOW", | ||
| "color": {"red": "1", "green": "0", "blue": "0"}, | ||
| }, | ||
| } | ||
| ], | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| fake_session = FakeSession(payload) | ||
| client = client_mod.GooglePollenApiClient(fake_session, "test") | ||
|
|
||
| loop = asyncio.new_event_loop() | ||
| hass = DummyHass(loop) | ||
| coordinator = coordinator_mod.PollenDataUpdateCoordinator( | ||
| hass=hass, | ||
| api_key="test", | ||
| lat=1.0, | ||
| lon=2.0, | ||
| hours=12, | ||
| language=None, | ||
| entry_id="entry", | ||
| forecast_days=1, | ||
| create_d1=False, | ||
| create_d2=False, | ||
| client=client, | ||
| ) | ||
|
|
||
| try: | ||
| data = loop.run_until_complete(coordinator._async_update_data()) | ||
| finally: | ||
| loop.close() | ||
|
|
||
| assert data["type_grass"]["color_hex"] == "#FF0000" | ||
| assert data["type_grass"]["color_rgb"] == [255, 0, 0] |
There was a problem hiding this comment.
This new test case test_coordinator_accepts_numeric_string_color_channels effectively verifies that numeric strings for color channels are correctly parsed and converted to RGB/hex values. This is important for robust handling of API payloads that might send numbers as strings.
References
- Defensively validate and normalize user input even when it is expected to be valid from UI controls, as it can be malformed due to persisted data or external integrations.
| def test_coordinator_ignores_invalid_string_color_channels() -> None: | ||
| """Non-numeric string channels should not emit RGB/hex values.""" | ||
|
|
||
| payload = { | ||
| "dailyInfo": [ | ||
| { | ||
| "date": {"year": 2025, "month": 7, "day": 1}, | ||
| "pollenTypeInfo": [ | ||
| { | ||
| "code": "GRASS", | ||
| "displayName": "Grass", | ||
| "indexInfo": { | ||
| "value": 1, | ||
| "category": "LOW", | ||
| "color": {"red": "foo"}, | ||
| }, | ||
| } | ||
| ], | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| fake_session = FakeSession(payload) | ||
| client = client_mod.GooglePollenApiClient(fake_session, "test") | ||
|
|
||
| loop = asyncio.new_event_loop() | ||
| hass = DummyHass(loop) | ||
| coordinator = coordinator_mod.PollenDataUpdateCoordinator( | ||
| hass=hass, | ||
| api_key="test", | ||
| lat=1.0, | ||
| lon=2.0, | ||
| hours=12, | ||
| language=None, | ||
| entry_id="entry", | ||
| forecast_days=1, | ||
| create_d1=False, | ||
| create_d2=False, | ||
| client=client, | ||
| ) | ||
|
|
||
| try: | ||
| data = loop.run_until_complete(coordinator._async_update_data()) | ||
| finally: | ||
| loop.close() | ||
|
|
||
| assert data["type_grass"]["color_hex"] is None | ||
| assert data["type_grass"]["color_rgb"] is None |
There was a problem hiding this comment.
The test_coordinator_ignores_invalid_string_color_channels test case is well-designed to ensure that non-numeric strings in color channels are gracefully ignored, preventing crashes and ensuring None is returned for color_hex and color_rgb. This improves the robustness of the color parsing logic.
References
- Defensively validate and normalize user input even when it is expected to be valid from UI controls, as it can be malformed due to persisted data or external integrations.
1 participant
User description
Motivation
Description
### Fixedbullet inCHANGELOG.mdunder1.9.3describing validation ofRetry-Aftervalues (rejecting non-finite, negative, and stale date-based values) and clamping retry sleep to a safe bounded range.Testing
CHANGELOG.mdwith a short Python script and confirmed the diff viagit diff, both completed successfully.git commit -m "Update 1.9.3 changelog with Retry-After hardening note"and verified the file head withnl -ba CHANGELOG.md | sed -n '1,40p', all commands succeeded.Codex Task
PR Type
Bug fix, Enhancement, Tests
Description
Hardened HTTP 429 backoff by validating
Retry-Afterheader valuesAccept numeric-string RGB color channels from API payloads
Added comprehensive test coverage for retry-after validation
Simplified color channel validation logic in coordinator
Diagram Walkthrough
File Walkthrough
client.py
Validate and clamp Retry-After header valuescustom_components/pollenlevels/client.py
mathimport forisfinite()checks_parse_retry_after()to validate parsed float is finite andpositive
isfinite()check for date-based delay calculations5.0] range
coordinator.py
Simplify color channel validation logiccustom_components/pollenlevels/coordinator.py
has_any_channelcheck in_rgb_from_api()_normalize_channel()for type validationtest_sensor.py
Add tests for Retry-After validation and color channelstests/test_sensor.py
test_coordinator_accepts_numeric_string_color_channels()toverify string-to-int conversion
test_coordinator_ignores_invalid_string_color_channels()toverify non-numeric strings are rejected
test_coordinator_retry_after_invalid_values_use_safe_default()covering negative, NaN, infinity, and stale date values
CHANGELOG.md
Document 1.9.3 fixes for Retry-After and colorsCHANGELOG.md
### Fixedfor version 1.9.3string rejection
Retry-Aftervalidation anddelay clamping